Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Registered prop as ref / [refProp]={value} and [customEvent]={handler} #28

Closed
wants to merge 11 commits into from

Conversation

dantman
Copy link

@dantman dantman commented Mar 4, 2018

I'm new to the RFC process, I hope I drafted this well. There is probably some room for better names or glossary terms to use.

facebook/react#7901 and other custom attribute/property discussions like #15 and facebook/react#11347 are relevant to this, but not conflicting.

This could be used as an alternative way of handling our custom event issue. But I still think it's worth implementing one of the other proposals. This API could be used for more advanced stuff.

@streamich
Copy link

streamich commented Mar 4, 2018

Would be nice to generalize this mechanism as this problem of "splitting props in groups" is not relevant only for DOM elements. For example, when using various CSS-in-JS solutions — like styled components or jsxstyle — there are often three groups or props that a styled component can receive:

  • Logical component props
  • CSS overrides
  • DOM element attributes

As an example, consider a styled component, which is a <button> with a customisable border colour:

const Button = styled('button')(props => ({
  border: `1px solid ${props.primary ? 'blue' : 'grey'}`
}));

Now I use it:

<Button primary width="100px" aria-label="Some button">
  Click me!
</Button>

Above you can see all three groups of props:

  • Logical prop [primary], which is only used in the CSS template.
  • CSS override width="100px" that is added to the CSS template dynamically.
  • Ref attribute aria-label="Some button" that has to go on to the <button> element directly.

More reading:

@streamich
Copy link

streamich commented Mar 4, 2018

Maybe there could be some mechanism to prefix, or "namespace", any prop, that way all props would go into their groups automatically, something like:

<Button primary css.width="100px" attr.aria-label="Some button">
  Click me!
</Button>

Above would be equivalent to

createElement(Button, {
  primary: true,
  css: {
    width: '100px',
  },
  attr: {
    'aria-label': 'Some button',
  },
}, 'Click me!')

@dantman
Copy link
Author

dantman commented Mar 4, 2018

@streamich IMHO that's a bit of a separate issue to the advanced things described in the RFC. But I'm willing to discuss it.

One problem I have with the css.width idea exactly as-is, is that it now takes a single character to de-optimize PureComponent. In JSX css.width="100px" looks like you are defining flat props, but in reality it's actually css={{width: "100px"}}, which creates a new object every render and doesn't allow PureComponent to work.

If we wanted that, IMHO we'd have to come up with some way to memoize the prop code in a way that avoids creating a new value when css.* is shallow equal.

I have a feeling that what you're describing and what I'm aiming for in the RFC are different issues worth different RFCs and solutions.

However I will note that I did call the registry CustomPropRegistry and only include references to "ref prop" in the registerRefProp method. i.e. I left room for future extensibility for non-ref use cases. For example, it's not what you're looking for but there is room for something like const cssWidth = CustomPropRegistry.registerStyleProp('width'); and <Button [cssWidth]='100px' /> which would be used by ReactDOM to set the style.width without using style={{}}, though I'm not completely certain about that one and it's interactions.

Perhaps if you're aiming to fit this into an extension of my proposal, you could have "namespaced" properties registered for some library. So you could write <Styled.Provider><Button [css('width')]='100px' /></Styled.Provider>.
css would be a function something like export const css = memoize((propName) => StyledPropRegistry.registerNamespacedProp(cssNamespace, propName));.
namespace would be the result of const cssNamespace = StyledPropRegistry.createNamespace('styled.css'); the 'styled.css' just being a descriptive name, not actually used for anything other than perhaps the descriptive name in the Symbol (i.e. if you looked at the symbol in the console for the prop name you'd see "Symbol('styled.css.width')".
And we'd probably have a helper on the namespace like cssNamespace.extractProps(props) which would extract the namespaced props (i.e. cssNamespace.extractProps({[css('width')/*Symbol('styled.css.width')*/]: '100px'}) => {width: '100px'}) and allow styled to use those props.

@streamich
Copy link

@dantman I'm not sure I like the namespace idea myself, that's just a quick idea.

@dantman
Copy link
Author

dantman commented Mar 6, 2018

As an extra note, I've kind of glossed over providing any rationales for adding the [computed]={value} syntax in JSX. This is because I don't really consider adding computed property syntax to JSX to be part of this RFC itself.

I'm not proposing some custom syntax for this RFC, computed property syntax is part of the language itself; all I'm proposing is that the JSX syntax be updated so it properly includes a piece of ES2015 syntax that wasn't added yet.

Even without this RFC I believe that the computed property syntax could still be useful to people for other purposes.

Sometimes you may want to use computed properties as you use them in object literals, to set an attbute whose name varries for some reason.

const Field = ({type, value, ...props}) => {
  let Tag = type;
  let valueProperty = 'value';

  if ( type === 'select' ) {
    return <SelectField {...props} value={value} />;
  }

  if ( type === 'textarea' ) {
    Tag = 'textarea';
    type = undefined;
  }

  if ( type === 'checkbox' || type === 'radio' ) {
    valueProperty = 'checked';
  }

  return (
    <Tag
      {...props}
      type={type}
      [valueProperty]={value} />
  );
};

Or, perhaps some framework or library would like to decide that props with string keys are "user data" and not define meaning for them. But has some special case props that they need to handle and would like to export a Symbol to use instead of reserving a named property for that purpose.

// some-library/index.js
// ...
export const importantRef = Symbol('importantRef');
export class SomeComponent extends PureElement {
  _setImportantRef = (ref) => {
    if ( this.props[importantRef] ) this.props[importantRef](ref);
  };
  render() {
    this.id = this.id || uuid();
    return (
      <div className='something'>
        <div className='nested-deeply'>
          <important-element ref={this._setImportantRef} />
        </div>
      </div>
    );
  }
}

// some-application.js
import {importantRef, SomeComponent} from 'some-library';

export default () => (
  <SomeComponent
    [importantRef]={(ref) => { ... }}
    myProp='foo' />
)

@gaearon
Copy link
Member

gaearon commented Aug 18, 2021

Hey @dantman! I really appreciate how detailed your writeups are, and I'm sorry we haven't been responsive. I've wanted to reply to this a few times, but it felt like writing up a reply that's as detailed would take me days, and anything less would be unfair to your effort. However, as noted in #182, this is probably the wrong way to approach it. So I'll write up my thoughts briefly.

There are a few things that make this proposal unlikely. The first is the reliance on syntax. Like you said, it's not technically coupled to the syntax change, but without the syntax, the proposal seems to lose its shine. But changing or adding syntax is a very high bar. Aside from the DOM cases (where arguably it's just due to unfortunate prop design), it's very unusual to want to dynamically specify a prop name. I would say it's usually an anti-pattern. With strong typing it's even less likely that you'd want to do that. So the JSX proposal doesn't feel like it stands on its own, and that in turn seems to make this proposal non-viable.

Setting syntax aside, there are a few other things. One is the notion of "registries". This isn't very composable. While you've tried to tackle this problem with provider-like API, it feels disproportionate to me that using a component that cares about some event would require wrapping my app in another provider. This doesn't compose very nicely if I just want to put something on npm. Granted, it's an existing problem for things like themes, but at least there it's for some level of control (like switching the theme). Also, what happens if I forget the provider?

This proposal is pretty heavy on new concepts around imperative features and escape hatches. Like the idea of "ref props" and how they work. We'd like to get people to use refs less (when possible), not more. So this is unfortunate.

Custom attributes were already very common on the web, which is why we've opened up the list. But note that we still introduced a heuristic instead of a separate syntax. I think your proposal is more in line with libraries that differentiate attributes and properties. We try to keep it a single concept because in the React custom component world, there is no distinction. As your app grows, you will use custom React components more and more, so we try not to add concepts or syntax that's only really useful at the lowest level. Things we add should ideally "pay for themselves" as the app scales and be useful for custom components too. Things that are only useful for the lowest level should ideally be very contained (example: #96) and don't interfere with other features or concepts or syntax too much. Most of the React experience is optimized for <Button />, not <button />.

New events aren't added that often. When they're added, we do want to add them to the list anyway. This is because we care about built-in event semantics, like inferring event priority. So in practice we wouldn't be able to get rid of that list. For any built-in standard event, we'd want to provide the canonical React name.

Custom events used for custom elements are a different story. However, that is a more niche use case in general than regular DOM elements. We're currently planning to approach them similarly as what Preact does, relying on a heuristic. There's an ongoing discussion in facebook/react#11347 where the latest plan is being shared and iterated on.

The last use case you bring up regarding gestures is interesting. The current status quo there is to use Custom Hooks and some kind of "ref merging" mechanism to combine the function refs they return. This is definitely clumsy and has been a source of some frustration. However, I would suggest to look at tackling that separately.

@gaearon gaearon closed this Aug 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants